Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dark snapshots to storybook tests #4928

Merged
merged 2 commits into from
Sep 25, 2024
Merged

Conversation

obulat
Copy link
Contributor

@obulat obulat commented Sep 13, 2024

Fixes

Related to #4305 by @zackkrida

Description

This PR adds the dark snapshots to the Storybook tests.

It also adds a 100ms wait after taking the light snapshot, before taking the dark mode snapshot, to make sure that the dark theme has fully rendered (otherwise, the white line underneath the filters tabs was saved as anywhere between gray and white, depending on how much of the theme switching was finished by the time of the snapshot).
The "slim-filled-borderless" focus test was removed since we no longer use this class.

Testing Instructions

Look through the added snapshots. Only dark mode storybook snapshots should be added. Some elements might need fixing, but not in scope of this PR. The CI should pass.

Checklist

  • My pull request has a descriptive title (not a vague title likeUpdate index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.
  • I ran the DAG documentation generator (ov just catalog/generate-docs for catalog
    PRs) or the media properties generator (ov just catalog/generate-docs media-props
    for the catalog or ov just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@obulat obulat changed the base branch from main to enable-color-theme-snapshots September 13, 2024 18:36
@openverse-bot openverse-bot added 🧱 stack: frontend Related to the Nuxt frontend 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work 🏷 status: label work required Needs proper labelling before it can be worked on labels Sep 13, 2024
@obulat obulat added 🟨 priority: medium Not blocking but should be addressed soon 💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users and removed 🏷 status: label work required Needs proper labelling before it can be worked on 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Sep 13, 2024
@obulat obulat force-pushed the add/storybook-dark-snapshots branch 2 times, most recently from d627e9b to 813db9f Compare September 14, 2024 06:12
@obulat obulat force-pushed the enable-color-theme-snapshots branch 2 times, most recently from 82e3c7a to 6bcc141 Compare September 15, 2024 10:54
@obulat obulat force-pushed the enable-color-theme-snapshots branch 2 times, most recently from bf04a21 to 27dc277 Compare September 16, 2024 12:30
@obulat obulat force-pushed the enable-color-theme-snapshots branch 2 times, most recently from bd11e2e to effb9d2 Compare September 18, 2024 06:36
Base automatically changed from enable-color-theme-snapshots to main September 19, 2024 08:53
@obulat obulat force-pushed the add/storybook-dark-snapshots branch 4 times, most recently from ec33781 to 6bf0412 Compare September 20, 2024 08:51
@obulat obulat self-assigned this Sep 20, 2024
@obulat obulat marked this pull request as ready for review September 20, 2024 09:13
@obulat obulat requested a review from a team as a code owner September 20, 2024 09:13
@obulat obulat force-pushed the add/storybook-dark-snapshots branch 2 times, most recently from 60c51ae to 394a116 Compare September 23, 2024 18:51
Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for the two sleeps. Left a comment asking for clarification.

Comment on lines 97 to 99
// Wait for the theme to change.
await sleep(100)

// Wait for the theme to change.
await sleep(100)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional to have two sleeps? It's unfortunate to need to use one at all... could we instead do something in turnOnDarkMode that probes the page for the expected classname on the root element?

If we absolutely need two separate sleeps, then it would be better to document them explicitly in reference to each other (and maybe move them into turnOnDarkMode, if it's always needed anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two sleeps are caused by the faulty merge conflict resolution :( I'm removing the extra one.
The existence of class name does not guarantee that the rendering has caught up and all colors have changed. I've seen a range of white-to-gray colors in the separator lines in the flaky snapshots when working on this, so I think a sleep is the best solution here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, understood.

I'm surprised it doesn't happen in a single rendering pass 🤔. What causes the browser to take multiple passes to render in response to the classname change, I wonder.

@obulat obulat force-pushed the add/storybook-dark-snapshots branch 2 times, most recently from 1fdba95 to af8ed47 Compare September 25, 2024 13:41
Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, it makes sense that fixing component issues it outside the scope of this PR. Just because this was a good point at which to review them (all in one PR, easy to find) I looked through the snapshots to see if anything should be noted as needing to fix later. Nothing obviously wrong is there, but perhaps @zackkrida or @dhruvkb are better suited to at least glance through them to make sure. The goal being just to note any issues so we can fix them later.

In any case, it doesn't block merging this PR, review of the snapshots can happen afterwards 🙂

@obulat obulat merged commit af8b89f into main Sep 25, 2024
44 checks passed
@obulat obulat deleted the add/storybook-dark-snapshots branch September 25, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: frontend Related to the Nuxt frontend
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants